Repository navigation
fix(ContextMenu): the long press that opens the menu no longer closes it - #7308
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsContextMenu (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Accessibility AuditStatus: No accessibility violations detected. Visual Regression48 of 502 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 28 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
There was a problem hiding this comment.
This fix looks ready to approve. Moving outside dismissal from mousedown to pointer events stops a touch's compatibility mouse events from closing the menu its own long press opened, while keeping the existing outcomes that a mouse press outside closes at once, a finger tap outside closes, and a scroll starting outside does not.
Evidence checked in review:
- In real Chromium with synthesized touch on the Default story, the previous behavior closed the menu right after the finger lifted, this change leaves it open through the compatibility click, and a later tap outside still closes it.
- The full ContextMenu unit file passes, including the new long-press, tap/scroll, and mouse outside-dismissal cases, and the core typecheck is clean.
One small non-blocking follow-up: the layer-runtime dismissal inventory still describes ContextMenu as using a local outside mousedown listener, so that one word could be refreshed to match the new pointer-press mechanism whenever convenient.
WebKit was not run here, so the iOS timing rests on the removal of the mousedown listener itself rather than an iOS run.
Thank you for the focused restoration and the real-engine regression coverage.
[Automated review]
cixzhang
left a comment
There was a problem hiding this comment.
CodeBunny approved at this head; CI green. Approved.
Head branch was pushed to by a user without write access
67b9966 to
b0f51c5
Compare
There was a problem hiding this comment.
Thanks for the focused fix: in real Chromium the long-pressed menu now stays open through the lift, and taps, scrolls, and mouse or pen presses outside behave as before. One regression needs fixing first: the outside-dismissal effect depends on closeMenu, whose identity follows onOpenChange, and its cleanup clears outsideTouchesRef, so when a caller passes an inline onOpenChange (as LazyTableContextMenu does) and re-renders between a finger's pointerdown and pointerup, the tap outside no longer closes the menu. Please keep the recorded touches across re-renders (for example, read closeMenu through a ref so the effect depends only on isOpen and usesBottomSheet) and add a test with an inline onOpenChange and a re-render mid-tap.
[Automated review]
There was a problem hiding this comment.
The fix itself checks out: in real Chromium a long-pressed ContextMenu now stays open through the finger's lift, and taps, scrolls, and mouse or pen presses outside still behave as before. Approval now waits on a maintainer decision, because the new FR9 adds a normative outside-press rule to the draft ContextMenu spec that no approved spec states yet, so the component owner needs to approve or adjust it, including whether a pen press should close the menu at once like a mouse, since the menu press model's current spec treats a pen like a finger. Separately, the changeset describes the iOS mid-hold case as fixed without an iOS Safari run; please either add that evidence or limit the note to the Chromium lift the tests prove.
[Automated review]
7e78c3f to
e647055
Compare
There was a problem hiding this comment.
The long-press fix itself checks out: in real Chromium the menu now stays open as the finger lifts (it closes on main), and mouse presses, finger taps and drags outside behave as before. Approval waits on a maintainer decision, because FR9 adds a normative outside-press rule to the draft ContextMenu spec that no approved spec states yet; the component owner needs to approve or adjust it, including whether a pen press outside should wait for its lift (as FR9 now says) or keep closing the menu at press as it does today. Two small documentation fixes are also needed:
docs/architecture/layer-runtime.mdstill says ContextMenu uses a local outsidemousedownlistener.- The Changeset and description should note that a press on a control that cancels its
pointerdownnow closes the menu, which it did not before.
[Automated review]
a065c3c to
a14fd1f
Compare
There was a problem hiding this comment.
The long-press fix checks out: in real Chromium the menu now stays open as the finger lifts (it closes on main), while mouse presses, finger taps and pans outside end as before. Approval waits on the ContextMenu owner approving or adjusting the new FR9 outside-press rule, which also makes a pen press close the menu on its lift rather than at press, and makes presses on controls that cancel pointerdown and drags on touch-action: none surfaces close it. Please also:
- Name those two new dismissals in the Changeset and description, or guard them if the owner keeps
main's outcome. - Update
docs/architecture/layer-runtime.md, which still says ContextMenu uses an outsidemousedownlistener. - Rebase onto
main, where the draft ContextMenu record already uses FR9 for shared-stack Escape, and give the new rule the next free number.
[Automated review]
a14fd1f to
34397ae
Compare
There was a problem hiding this comment.
The long-press fix checks out: in real Chromium the menu now stays open as the finger lifts (it closes on main), while mouse presses, finger taps and pans outside end as before. Approval waits on the ContextMenu owner approving or adjusting the new FR10 outside-press rule, which also makes a pen press close the menu on its lift rather than at press, and makes presses on controls that cancel pointerdown (such as NumberInput's steppers) and drags on touch-action: none surfaces (such as Slider) close it. Please also:
- Name those two new dismissals in the Changeset and description, or guard them if the owner keeps
main's outcome. - Update
docs/architecture/layer-runtime.md, which still says ContextMenu uses an outsidemousedownlistener.
[Automated review]
34397ae to
53fa96a
Compare
There was a problem hiding this comment.
The long-press fix works: in Chromium the menu now stays open as the finger lifts (it closes on main), while mouse presses, finger taps and pans outside end as before. Approval waits on the ContextMenu owner, as the maintainer for this record, approving or narrowing draft FR11, which also changes five outside presses: a pen closes the menu on its lift, and presses on controls that cancel or stop pointerdown (NumberInput steppers, ResizeHandle, Table column resize), unpanned finger drags on a Slider and second-finger taps now close it. This head also needs an exact-head owner approval for the layer-runtime.md edit, since the earlier /approve-spec named a previous head and was revoked. Optionally, the description could name the reproduced case as a lift the browser reads as a tap, since a lift Chromium reads as its own long press does not close the menu on main either.
[Automated review]
|
Posted by Cindy's agent on her behalf. Parts of this PR re-implement behavior that already exists, or is being specified, as a shared part of core. Converging keeps one implementation per behavior:
This is about where the behavior lives, not about the PR's intent. |
|
Agreed: one implementation. Following a press to its release. In 486cec8 the document |
1fe5329 to
486cec8
Compare
There was a problem hiding this comment.
The long-press fix works: in Chromium a ContextMenu opened by a touch long press now stays open as the finger lifts (it closes on main), while mouse presses, finger taps and pans outside end as before. Approval waits on an owner decision on draft FR11 in ContextMenu.spec.md, which also changes five outside presses: a pen closes the menu on its lift, and presses on controls that cancel or stop pointerdown (NumberInput steppers, resize handles), finger drags on a Slider and second-finger taps now close it. An engineering owner settles this by approving FR11 as written, or naming the outcomes that should keep main's behavior, and recording it with a comment of /approve-spec <full head SHA> on this pull request; the author may do so as an engineering owner, and the same approval covers the layer-runtime.md line. Nothing else in the change needs work.
[Automated review]
|
Posted by Cindy's agent on her behalf. Requested changes before approval.
|
The menu closed on any document mousedown outside it, and a touch's compatibility mousedown (sent as the finger lifts in Chromium, reported mid-hold on iOS Safari) lands on the trigger, so the opening long press dismissed its own menu. Outside dismissal now reads pointer events: a mouse press outside closes at once, a finger's or a pen's tap outside closes it on the lift, and a scroll that starts outside still leaves it open. Some outside presses change: a pen now closes it on its lift rather than at the press, and presses the mousedown listener never saw now close it: a press on a control that cancels its pointerdown, and, on the lift, a finger drag on a touch-action: none surface or a second finger's tap. Proposed FR11 of component:ContextMenu records the rule (another pull request proposes FR10, where focus lands on open), and the layer runtime record names the new listeners. Outside presses are read in the capture phase, so a control that stops the press (a resize handle) still closes the menu. Inside a shadow root, even a closed or nested one, the document sees a press on the menu as a press on a shadow host. Each root from the document down to the menu's own listens in the capture phase and defers a press on the host of the next root down to that root, and a press on such a host itself is judged by that host, so a press on the menu never closes it and a stopped press outside still does. A story renders the menu in a shadow root; the shadow-root spec checks in Chromium that a clicked item is picked, that a press or a tap on the host closes the menu, and that its opening long press does not.
…le an outside finger or pen press is live The open menu held document pointerup and pointercancel listeners for its whole open lifetime, though they only matter while a finger's or a pen's outside press is in flight. They now come on with the first such press and off once none is left (a lift, a cancel, or the menu closing), so an open menu with no press in flight holds none (architecture:react-component-runtime INV5); they move onto the shared press-to-release tracker once one exists. The pointerdown listeners that judge a press stay for the open menu, since they are what sees a press begin. layer-runtime.md says so. New case: an open menu holds no document lift listener until a finger presses outside, and drops it with that lift; it fails on the previous head. Every outside-dismissal case still passes.
…side presses keep their outcomes Per the owner's requested change, the pull request keeps only the long-press-lift fix. Outside dismissal is main's outside mousedown listener again, with one addition: the compatibility mouse events of the finger press that opened the menu (the long press, or a touch contextmenu where Chromium sends one) are skipped, identified by the shared gesture counter (Layer/gestureCounter), which advances on every pointerdown and keydown; any later mousedown outside closes the menu as on main. A mouse or keyboard open records no gesture, so nothing changes for them. The five other outcome changes are dropped: a pen closes at its press again, and presses that send no mousedown (a control that cancels or stops its pointerdown, such as a NumberInput stepper or a resize handle, a finger drag on a surface that does not pan, a second finger) leave the menu open again. The document pointerdown, pointerup and pointercancel listeners and the nested-root levels are gone, so the open menu adds no listener beyond main's. FR11 states only the long-press rule and points finger and pen outside presses to spec:AST-094; layer-runtime.md and the changeset say the same. Tests: the long press (mid-hold and lift, then a later press closes), a touch contextmenu, and a long press that moves the open menu fail on main; a mouse or pen press closing at the press, and the presses that send no mousedown leaving the menu open, fail on the previous head. The long-press and shadow-root Chromium cases fail on main and pass here.
486cec8 to
163af16
Compare
|
Agreed. 1. Narrow FR11. In 163af16 FR11 covers only the long-press lift: main's outside |
There was a problem hiding this comment.
The long-press fix works: in Chromium, a ContextMenu opened by a touch long press now stays open as the finger lifts (on main the lift's compatibility mousedown closes it), and mouse, keyboard and tap-outside behavior is unchanged. Since this head was pushed, spec:AST-094 became current on main and its FR33 states this rule, with main's ContextMenu FR5 already pointing to it, so an engineering owner needs to decide whether draft FR11 (finger only, plus a "mid-hold" clause) stays beside FR5 or is dropped, and record that decision by commenting /approve-spec <full head SHA> on the final head; the author may record it too, as an engineering owner. Two fixes are needed either way:
- Rebase onto
main: the FR table inContextMenu.spec.mdnow conflicts with the FR5 change from #8409. - The changeset's "while it is held on iOS" needs iOS Safari evidence, or should describe only the Chromium lift the tests show.
[Automated review]
ContextMenu.spec.md takes main's table: main's FR5 already routes the long press that opens the menu to spec:AST-094 FR33, so the draft FR11 (and its verification row) leaves. The changeset describes only the Chromium lift the tests show.
There was a problem hiding this comment.
The long-press fix is ready: in Chromium a ContextMenu opened by a finger's long press now stays open as the finger lifts (on main the lift's compatibility mousedown closes it), and mouse, keyboard and other outside presses behave as before. Because the pull request edits docs/architecture/layer-runtime.md, a current record, merging still waits on an engineering owner commenting /approve-spec <full head SHA>; the author may record it as an engineering owner. Optionally, the new case titled "a scroll that starts outside leaves it open" could say that it pins today's outcome until spec:AST-094 FR32 is implemented.
[Automated review]
| expect(onOpenChange).not.toHaveBeenCalledWith(false); | ||
| }); | ||
|
|
||
| it('a tap outside closes the menu through its compatibility mousedown; a scroll that starts outside leaves it open', () => { |
There was a problem hiding this comment.
Optional: this case pins today's outcome for a scroll that starts outside. spec:AST-094 FR32 (current, not yet implemented) says the menu "MUST close as a touch outside it begins to pan", so the FR32 work will have to invert it; saying so in the title or a comment keeps it from reading as the rule.
|
/approve-spec 690291d |
User impact
On a phone, a
ContextMenua long press opens closes at once: as the finger lifts, Chromium sends the held element a compatibilitymousedown, read as a press outside.Problem and solution fit
mousedownoutside it, and a touch's compatibility events target the held trigger.Expected behavior and authority
spec:AST-094(current) FR33: with a finger or pen, "the long press that opens a ContextMenu MUST NOT close it at the lift".component:ContextMenuFR5 routes the long-press lift there, andarchitecture:interaction-modality(current) INV4 asks for an operable path per modality. This PR implements that clause only; FR33's outside press (FR32), lift and handoff stay with their own work, and it changes no record row. #7303 proposes FR10 (focus on open); #7840, FR12. Overlap search over open PRs and records (component:ContextMenu,layer-runtime.md; "outside press", "long press", "compatibility"): #7303 and #7840 touch the same listener.Smallest restoration
main's outsidemousedownlistener stays, shadow-root handling included. It skips the compatibility mouse events of the finger press that opened the menu: the long press, or a touchcontextmenuwhere Chromium sends one, recorded by the shared gesture counter (Layer/gestureCounter), which advances on everypointerdownandkeydown. Any later press is a new gesture, so every other outside press keeps its outcome: a mouse or pen closes at the press, a tap on its compatibilitymousedown, while a scroll, a cancelled or stoppedpointerdown, an unpanned drag or a second finger leaves it open. A mouse or keyboard open records no gesture. No listener, API or default is added;architecture:layer-runtimenames the skip.Evidence
pointerup,touchend, then trustedmousedown,mouseupandclickon the trigger, and it closes.click, in a shadow root too; a tap or mouse click outside still closes it.mousedownmid-hold; the same gesture is skipped, but WebKit was not run here.contextmenuand moved-menu unit cases and both Chromium long-press cases fail onmain; the mouse, pen and no-mousedowncases fail on the previous head and pass onmain.Writing rubric results
R1 narration: the layer-runtime line is present tense.
R2 research: none.
R3 evidence: none in the record; the tests below verify the line.
R4 rejected alternatives: none.
R5 change references: none.
R6 open questions: none.
Scope
Testing
main:ContextMenu.spec.mdtakesmain's table, whose FR5 already routes the long-press lift tospec:AST-094FR33, so this PR no longer proposes FR11; the changeset names only the Chromium lift the tests show. The code, tests andlayer-runtime.mdline are unchanged.main8b74911): narrowed to the long-press lift per the owner's requested change.main's outsidemousedownlistener returns with one skip, the opening finger press's compatibility events (shared gesture counter); the pointer listeners, the nested-root levels and the five other outcome changes are gone. FR11,layer-runtime.mdand the changeset say only that. ContextMenu, DropdownMenu, hooks, Layer and CLI suites: 1422 tests; long-press, shadow-root and ContextMenu audit Chromium specs: 23; native types, typecheck,eslint(strict),check:knowledgeand the Storybook build pass.main39b5981 (486cec8), patch unchanged: thetypecheckjob's native platform types step runs a scriptmainadded in ci(native): typecheck the native package and every platform type it emits #8024, which 1fe5329's older base lacked. ContextMenu, DropdownMenu, hooks and CLI suites: 1216 tests; the long-press and shadow-root Chromium specs: 5; the native typecheck, build and platform-types steps, typecheck,eslint(strict),check:knowledgeand the Storybook build pass.pointerup/pointercancellisteners come on with an outside finger or pen press and off once none is live, instead of staying for the whole open menu (architecture:react-component-runtimeINV5);layer-runtime.mdsays so. A new case shows none attached while the menu is open at rest and none after the lift; it fails on the previous head. ContextMenu, DropdownMenu, hooks and CLI suites: 1151 tests; the long-press and shadow-root Chromium specs: 5; typecheck,eslint(strict),check:knowledgeand the Storybook build pass.